-
Notifications
You must be signed in to change notification settings - Fork 206
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: psmCharter.addInstance interface shape correction #6202
Conversation
The last two parameters are optional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.optional
is new to me but if it works for you locally then I believe it.
I added the bypass:integration
label. Since the test didn't catch the bug going it it shouldn't be affected by it being fixed ;-)
BrandShape, | ||
).returns(), | ||
addInstance: M.call(InstanceHandleShape, M.any()) | ||
.optional(BrandShape, BrandShape) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL .optional
. Where is it documented or implemented?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's very little documentation. I dunno where the implementations are. It's tested in test-heap-classes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From #6206 (comment) :
FWIW, this is implemented at
agoric-sdk/packages/store/src/patterns/patternMatchers.js
Lines 1537 to 1543 in 9b78628
optional: (...optArgGuards) => { | |
optionalArgGuards === undefined || | |
assert.fail(X`Can only have one set of optional guards`); | |
restArgGuard === undefined || | |
assert.fail(X`optional arg guards must come before rest arg`); | |
return makeMethodGuardMaker(callKind, argGuards, optArgGuards); | |
}, |
I'm actually surprised I didn't write any comments explaining this at all. I certainly agree it is needed.
Also rest
Description
Correct the interface shape declaration for
psmCharter.addInstance()
. The last two parameters are optional.Security Considerations
None
Documentation Considerations
None
Testing Considerations
Exercised by running
make scenario2-setup scenario2-run-chain-psm
.